Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Per object rid info for 3.2 #43830

Closed
wants to merge 1 commit into from
Closed

Per object rid info for 3.2 #43830

wants to merge 1 commit into from

Conversation

fire
Copy link
Member

@fire fire commented Nov 24, 2020

Made a pr branch for @lawnjelly to review per rid object info. Per rid object info is not trivial to port to Godot Engine master, but I wanted to share.

Partially supports godotengine/godot-proposals#63 by having individual object information.

image

Improvement notes:

  1. The calculation are wrong. Instancing isn't being subtracted.
  2. 2d render info isn't in.
  3. Write proposal

I don't expect this to be merged in 3.2.

Sponsored by IMVU.

@fire fire requested a review from reduz as a code owner November 24, 2020 18:23
@fire fire requested a review from lawnjelly November 24, 2020 18:23
@fire fire changed the title Per object rid. Per object rid info for 3.2 Nov 24, 2020
@fire fire force-pushed the per-rid-3-2 branch 2 times, most recently from f2ff701 to cb03acb Compare November 24, 2020 18:37
return new_info;
}

bool has_shadows() { return true; }
Copy link
Member Author

@fire fire Nov 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete has shadows.

Comment on lines -154 to -155
case RENDER_2D_ITEMS_IN_FRAME: return VS::get_singleton()->get_render_info(VS::INFO_2D_ITEMS_IN_FRAME);
case RENDER_2D_DRAW_CALLS_IN_FRAME: return VS::get_singleton()->get_render_info(VS::INFO_2D_DRAW_CALLS_IN_FRAME);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing 2d items.

Comment on lines -353 to -354
vp->render_info[VS::VIEWPORT_RENDER_INFO_2D_ITEMS_IN_FRAME] = VSG::storage->get_captured_render_info(VS::INFO_2D_ITEMS_IN_FRAME);
vp->render_info[VS::VIEWPORT_RENDER_INFO_2D_DRAW_CALLS_IN_FRAME] = VSG::storage->get_captured_render_info(VS::INFO_2D_DRAW_CALLS_IN_FRAME);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing 2d items.

}
render.vertices_count += s->array_len * amount;
storage->info.rid_render_info_render[s->mesh] = render;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this kind of thing it looks like it may be better just to store the counts in a temporary variable without an if,

int stats_vertex_count = 0;
stats_vertex_count += s->array_len * amount;

then have one if statement at the end of the function ..

if (storing_debug_stuff)
 ...

Just incrementing the counts isn't going to be much of a hit for 3d, even when stats are not being used, in fact the if statements may be more expensive than just doing the counts...

This way the stats would be less intrusive and easier to maintain.

info_write[VS::INFO_SURFACE_CHANGES_IN_FRAME] = info.snap.surface_switch_count;
info_write[VS::INFO_DRAW_CALLS_IN_FRAME] = info.snap.draw_call_count;
return new_info;
}
Copy link
Member

@lawnjelly lawnjelly Nov 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this simply be passed as a structure to fill in?
e.g.

get_captured_render_info(StatsRenderInfo &r_info)
{
r_info.objects_in_frame = BLAH;
...
}

or directly return the structure as a const ref?

Ah maybe this is for some thread protection or something. Still seems like redundant copying is going on.

@lawnjelly
Copy link
Member

lawnjelly commented Nov 25, 2020

I'm not actually so familiar with the 3d rasterizer, but don't we maintain stats already for the overall number of calls..., yes storage->info.render.vertices_count.

Perhaps an alternative paradigm could be to just note when we start on an object which is selected for stats, record the current storage->info.render.vertices_count, then when hitting an object that is not selected, record storage->info.render.vertices_count and find the difference? Could that work? (I'm not so familiar)

@lawnjelly
Copy link
Member

lawnjelly commented Nov 25, 2020

Overall though more render stats are welcome 👍 imo, I did a proposal on this too:
godotengine/godot-proposals#248

Myself I'm also particularly interested in being able to display non rendered stats such as the number of source verts (rather than the number of indices). I should edit that proposal to make that clear actually. The number of source verts gives an indication of the success of an import and whether vertices are being shared or too many are unique. This would not need to go via the rasterizer, it could be a query of the mesh instance itself.

You say you don't expect to be merged, but providing it can be made streamlined and as least intrusive as possible speaking for myself I could see this as beneficial in 3.2. The most important factor in my opinion is not having the stats complicate reading the rendering (in terms of verbosity or where the code is), or serve as a barrier to rearranging the rendering. But @clayjohn will chime in too I'm sure and we can ask reduz if you decide to go for it.

Also just to be clear as I haven't compiled this and tested, can this be used in game exports?

@fire
Copy link
Member Author

fire commented Dec 23, 2020

Since this PR is for 3.2 and I don't have the effort to pr this for master, I'm going to close the PR. It is salvageable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants